Adding a degree of freedom (dof) flag (--n-independent-echos) to allow users to set the dof used in fstat calculations #1177
Conversation
handwerkerd
left a comment
handwerkerd
left a comment
There was a problem hiding this comment.
Besides the one ongoing discussion about a warning message, the rest of this looks good.
We should definitely include some tests. I can help with those, if useful.
adding tests. simplifying getfbounds
|
@handwerkerd The tests/changes you made are in the latest commit. I also made a change to fix the fstat map test you added for echo_dof. One of the asserts is still failing in fstat map test. Its caused by the following failing. Should this test be strictly greater than 0? I think maybe the f_max may be preventing that. Can you take a look? |
|
Since the tests were passing on my local computer & failing here, I decided to push some commits to see if I could just solve it. I think the key problem was we have a maximum F value set to 500. That means, for a test >500, the F is a constant 500 and the difference between DOF==3 vs 5 is 0. Since the goal of the test was to make sure the change in DOF always changed the F stats, I needed to mask out the F==500 tests for both f_t2 and f_s0. |
|
@tsalo Do you want to take another look or is this good to merge? |
Co-authored-by: Taylor Salo <tsalo90@gmail.com>
Co-authored-by: Taylor Salo <tsalo90@gmail.com>
Co-authored-by: Taylor Salo <tsalo90@gmail.com>
Co-authored-by: Taylor Salo <tsalo90@gmail.com>
Co-authored-by: Taylor Salo <tsalo90@gmail.com>
Changed echo_dof to n_independent_echos
|
@handwerkerd some of the tests are failing. I merged your PR with my repo and made an update to get one of the unit tests to pass (changed np.concat to np.concatenate) and now a bunch of tests that were once passing are failing. Can you take a look? It seems to be due to an assertion you put in for the adaptive mask test. |
I think found the issue. A warning message in @tsalo Anything else to address before merging? |
Co-authored-by: Taylor Salo <tsalo90@gmail.com>
Co-authored-by: Taylor Salo <tsalo90@gmail.com>
Co-authored-by: Taylor Salo <tsalo90@gmail.com>
cleaned up f_thresh in generate_metrics
tsalo
left a comment
tsalo
left a comment
There was a problem hiding this comment.
The changes look good to me. I have one small suggestion to fix a typo. If you could merge that suggestion before merging this PR, that would be great. No need to request a fresh review after that.
One code comment that I haven't gotten clarification on is the claim that "EPTI has less dropout". Is that accurate or more a factor of having more echoes at shorter echo times? For example, for an echo at 40 ms, will EPTI show less dropout than EPI?
Co-authored-by: Taylor Salo <tsalo90@gmail.com>
|
@all-contributors please add @katielamar for code, design, ideas |
|
I've put up a pull request to add @katielamar! 🎉 |
3 participants
Hi everyone,
I've updated the tedana code to address issue #1168. I've made this a draft PR since this is my first time contributing to Tedana and I'm a bit new to submitting pull requests. Let me know if you have any suggestions on how I can improve the documentation/code.
Closes #1168.
Summary of Updates:
--n-independent-echosto set the degrees of freedom when running tedana workflow from the command lineecho_dofwhen calling thetedana_workflowfunction